Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #69 nanoplot error #70

Closed
wants to merge 2 commits into from
Closed

Conversation

drchriscole
Copy link

This PR stops bacass v2.0.0 from failing on the nanoplot step and fixes #69.

The newer version of NanoPlot v1.38.0 doesn't generate png image by default anymore meaning that the NANOPLOT process will always fail as the png output requirement will never be satisfied.

Removing png output requirement in NANPLOT process.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
    • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
    • If necessary, also make a PR on the nf-core/bacass branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@@ -23,7 +23,6 @@ process NANOPLOT {

output:
tuple val(meta), path("*.html"), emit: html
tuple val(meta), path("*.png") , emit: png
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could also make that one optional ...? with optional: true ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that optional output might be better, but thats just just a band aid here, so I think its fine.
The optimal solution might be to make NanoPlot output the png again? I mean it was sort of nice to look at them?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that static image generation is now dependent on having an internet connection, which cannot always be guaranteed. See: wdecoster/NanoPlot#281

The HTML reports are unaffected so still can visualise the plots. Making the pngs optional could be a pragmatic solution and doesn't cause the workflow to fail on occasions when internet access is unavailable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
No, an internet connection may not be a prerequisite to run the pipeline.
So I am fine with the PR ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree then too!

Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing linting test below is only due to not having the template update 2.2 integrated yet.
LGTM!

Edit: Hm, I am not able to merge the PR due to missing rights it seems (I expect that not everybody can merge it due to the failing linting tests, but I was hoping to use admin power; which I lack obviously, maybe for the better).

@Daniel-VM
Copy link
Contributor

I thin this PR is not needed anymore. nf-core template v2.9 has been updated in #86 and Nanoplot has been implemented via nf-core/modules where the *.png requirement has been solved by adding tuple val(meta), path("*.png") , optional: true, emit: png. Shall we close it?

@d4straub
Copy link
Collaborator

Indeed, thanks!

@d4straub d4straub closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants